feat(infra): concurrent materializer tests #1243
Conversation
LePremierHomme
changed the title
| @@ -59,13 +54,14 @@ fn read_manifest(file_name: &str) -> anyhow::Result<Manifest> { | |||
|
|
|||
| /// Parse a manifest file in the `manifests` directory, clean up any corresponding | |||
| /// testnet resources, then materialize a testnet and run some tests. | |||
| pub async fn with_testnet<F, G>(manifest_file_name: &str, alter: G, test: F) -> anyhow::Result<()> | |||
| pub async fn with_testnet<F, G>(manifest_file_name: &str, concurrency: Option<concurrency::Config>, alter: G, test: F) -> anyhow::Result<()> | |||
There was a problem hiding this comment.
- I consider this inversion of control (IoC) as convenient utility -- rather bespoke -- that the original developer of the materializer used to create an initial batch of tests. I would not glorify it and turn it into a focal entrypoint for every future test.
- There isn't a single clear cut way we'll want all potentially tests that require some concurrency to behave, so I'm not sold on introducing concurrency as framework feature.
Instead, I think we're better served if we introduced a non-IoC API here:
- Extract the logic that actually materializes the definition into a separate function that returns the Manifest, DockerMaterializer, DockerTestnet.
- The test can now call this function to materialize a definition, do whatever it wants (using whatever concurrency it desires).
- Something needs to have a drop guard here that destroys the materialized testnet, probably the DockerTestnet? Not sure if that's implemented.
There was a problem hiding this comment.
I've inverted the inversion, as you suggested, now providing a cleanup function. I don't think it's helpful to maintain 2 patterns, so I migrated prev tests.
It also helped to untangle concurrency from the framework, making it a simple test lib utility.
LePremierHomme
changed the title
LePremierHomme
changed the title
| } | ||
|
|
||
| pub async fn record(&mut self, label: String) { | ||
| let duration = self.start_time.unwrap().elapsed(); |
There was a problem hiding this comment.
I feel expect here if better if you assume caller know calling "start" should happen first.
There was a problem hiding this comment.
I'll revise this API once the reporting summary is more solid.
| Ok(bencher) => (Some(bencher), None), | ||
| Err(err) => (None, Some(err)), | ||
| }; | ||
| step_results.lock().await.push(TestResult { |
There was a problem hiding this comment.
if this triggers a lot of threads, then everyone is waiting on this as well, also as step_results gets big, so allocation might take some time. Just curious, if step_results are not updated at all, will there be a big difference?
There was a problem hiding this comment.
This is unlikely to be a bottleneck, it can only impose a small delay in the after-test lifecycle of the future, which isn't recorded nor is time sensitive. But I'll double check that once I'll get to high max concurrency figures.
|
|
||
| #[derive(Default)] | ||
| pub struct NonceManager { | ||
| nonces: Arc<Mutex<HashMap<H160, U256>>>, |
There was a problem hiding this comment.
I think this is a bottom neck as well, every address is waiting on the same lock. Maybe this might help: https://github.com/xacrimon/dashmap
There was a problem hiding this comment.
Yes, this is just a temporary solution, I was hoping to remove it entirely. If not, I'll optimize it.
Introducing concurrent tests in
materializer, enabling the generation of traceable workloads without significantly altering how test scenarios are written.Existing tests refactored to use
make_testnetinstead ofwith_testnet.Progress
docket_tests::benches::test_native_coin_transfer)simplecointransfer)NonceManagerwas introduced due toget_transaction_countnot being reliable. need to double check thatFor follow-up PRs